Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update docs for wallet standard #761

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikemaccana
Copy link
Contributor

@mikemaccana mikemaccana commented Mar 31, 2023

Update

This is a smaller PR that makes some aspects of the docs more apparent but, per discussion, doesn't mark individual wallet adapters as deprecated.

  • State what wallet adapter and wallet standard are in the README
  • Mention pnpm 8 requirement explicitly (I'm relatively sure pnpm 7 won't work)
  • Make Starter Projects more prominent by moving it before the long list of individual wallet adapters
  • Make demo and demo code more prominent in README
  • Use relative links wherever possible (which keeps links working across forks and looks neater)
  • Use a neater syntax so the markdown is viewable in terminal, VScode, and other apps that do not render markdown.

See what the docs look like after this PR is merged

Original comment

  • Rewrite docs to focus on wallet-standard, and note that wallet adapters for individual wallet apps are deprecated
  • Make demo and demo code more prominent in README
  • Move deprecated individual wallet adapter links into DEPRECATED.md
  • Move current instructions from PACKAGES.md and WALLET.md (now that PACKAGES is much smaller) to main README
  • Add notes that some community packages need updated for wallet-standard
  • Use relative links wherever possible (which keeps links working across forks)
  • Use a neater syntax (lists rather than markdown tables) so the markdown is viewable in terminal, VScode, and other apps that do not render markdown.
  • Remove APP.md as it doesn't use wallet-standard (we have link to demo code for something that does use wallet-standard)

@changeset-bot
Copy link

changeset-bot bot commented Mar 31, 2023

⚠️ No Changeset found

Latest commit: 5844e2d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@socket-security
Copy link

socket-security bot commented Mar 31, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No dependency changes detected in pull request

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

@mikemaccana mikemaccana force-pushed the master branch 2 times, most recently from 6016141 to 9b7ceb0 Compare March 31, 2023 18:11
@jordaaash
Copy link
Collaborator

Thanks for the initiative. I did a quick glance over this, but will need to do a more complete review. The adapters have not been deprecated, so I think this may be jumping the gun a bit. Some adapters are for wallets that need to load a third party library. There are also commonly-used wallets like Solflare which have not implemented the Wallet Standard, and others like Phantom which have, but the adapter provides functionality on iOS that isn't handled by MWA.

I imagine most of these adapters could be refactored so that the app instead loads a library which registers a Standard Wallet with the app, and there's no need for the wallets array of adapters passed to the WalletProvider. We could refactor and deprecate these adapters one by one, but this is a bigger effort than a doc change. Overall I think it's a good idea, but we don't want to deprecate support for a bunch of wallet adapters without pointing to an immediately useful alternative.

@mikemaccana
Copy link
Contributor Author

mikemaccana commented Mar 31, 2023

Got it - that sounds completely reasonable re: Solflare not being ready and Phantom providing extra hardware support on iOS. It's a more nuanced situation than what I originally thought.

Will the instructions in APP.md that points towards importing specific named wallets from @solana/wallet-adapter-wallets mean the resulting dApp doesn't support other installed wallets (eg from registerWallet)?

My concern after using https://github.com/solana-labs/dapp-scaffold (which I know is a separate Solana Labs repo, but linked to from here) is that new dApps won't support wallet-standard, which makes it harder for new wallets to be created on Solana and also wallets from other chains to add Solana support.

@jordaaash
Copy link
Collaborator

Will the instructions in APP.md that points towards importing specific named wallets from @solana/wallet-adapter-wallets mean the resulting dApp doesn't support other installed wallets (eg from registerWallet)?

No, all Standard Wallets and Mobile Wallet Adapter are supported in the React WalletProvider automatically. Any adapters you instantiate and pass to provider don't change that.

It would be cool if you didn't have to pass any adapters at all though, and instead you just called something like e.g. registerTorus once, which would create a Wallet interface for Torus and call registerWallet with it, and then actually load the Torus libraries on the first connect call as we do in the adapter. This means that Torus would be able to take advantage of the full Wallet Standard feature API instead of the more limited adapter API.

It should be pretty straightforward to refactor most of the adapters to this. I'm all for (eventually) deprecating/EOL-ing all adapters that could use the Wallet Standard directly though, since they really should do that instead of making every app bundle their dependencies.

@mikemaccana mikemaccana force-pushed the master branch 4 times, most recently from 1ab8740 to b93af5e Compare April 5, 2023 12:06
This is a smaller PR that makes some aspects of the docs more apparent but, per discussion, doesn't mark individual wallet adapters as deprecated.

 - State what wallet adapter and wallet standard are in the README
 - Mention pnpm 8 requirement explicitly (I'm relatively sure pnpm 7 won't work)
 - Make Starter Projects more prominent by moving it before the long list of individual wallet adapters
 - Make demo and demo code more prominent in README
 - Use relative links wherever possible (which keeps links working across forks and looks neater)
 - Use a neater syntax so the markdown is viewable in terminal, VScode, and other apps that do not render markdown.
@mikemaccana
Copy link
Contributor Author

mikemaccana commented Apr 5, 2023

No, all Standard Wallets and Mobile Wallet Adapter are supported in the React WalletProvider automatically.

Ah it does say that in a comment in the code, I just missed it. Thanks!

I've replaced the changes here with a much smaller version that just adds some additional information about wallet adapter per our discussion, and doesn't mark individual adapters ads deprecated. It should hopefully be useful to anyone else implementing wallet adapter in future, see the updated PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants